Skip to content

Fix stale PR file fetch completion#12

Open
MkDev11 wants to merge 5 commits intoentrius:testfrom
MkDev11:fix/issue-11-stale-pr-file-fetch-completion
Open

Fix stale PR file fetch completion#12
MkDev11 wants to merge 5 commits intoentrius:testfrom
MkDev11:fix/issue-11-stale-pr-file-fetch-completion

Conversation

@MkDev11
Copy link
Copy Markdown

@MkDev11 MkDev11 commented May 8, 2026 •
edited
Loading

Summary

Prevent stale active fetch-pr-files jobs from marking newer PR generations complete.

This change ties PR file-fetch jobs to the expected headSha/baseSha, uses those values in the BullMQ job id, and only marks scoringDataStored=true when the current PR row still matches the job generation. If the final guarded update finds that its generation is stale, it clears the trusted completion flag and enqueues a current-generation fetch.

Related Issues

Closes #11

Type of Change

  • Bug fix
  • New feature
  • Refactor
  • Documentation
  • Other (describe below)

Testing

  • npm run format:check
  • npm run lint
  • npm run build
  • git diff --check

Checklist

  • I have read the Contributing Guide
  • Code builds without errors
  • New and existing tests pass (if applicable)
  • Documentation updated (if applicable)
  • No unnecessary dependencies added

@xiao-xiao-mao xiao-xiao-mao Bot added the bug Something isn't working label May 8, 2026
anderdc
anderdc requested changes May 8, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@anderdc anderdc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no tests

anderdc
anderdc requested changes May 8, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@anderdc anderdc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no tests

@MkDev11
Copy link
Copy Markdown
Author

MkDev11 commented May 8, 2026

Addressed: added prebuild: npm test so the existing Build workflow runs the regression tests before nest build. Local npm run build, npm run lint, and npm run format:check pass.

@MkDev11 MkDev11 requested a review from anderdc May 8, 2026 21:03
@anderdc
Copy link
Copy Markdown
Collaborator

anderdc commented May 10, 2026

To clarify the prior "no tests" — this repo doesn't accept tests in contributor PRs. Drop packages/das/src/queue/pr-files-generation.spec.ts and revert packages/das/package.json (remove the new test script and the prebuild: npm test line). The queue/processor changes themselves are fine.

@MkDev11
Copy link
Copy Markdown
Author

MkDev11 commented May 10, 2026

To clarify the prior "no tests" — this repo doesn't accept tests in contributor PRs. Drop packages/das/src/queue/pr-files-generation.spec.ts and revert packages/das/package.json (remove the new test script and the prebuild: npm test line). The queue/processor changes themselves are fine.

updated

anderdc
anderdc requested changes May 10, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@anderdc anderdc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slim this down. The unique jobId change solves the actual dedup bug; the row-lock and the generation re-checks scattered through the fetch path defend against a rarer concurrent-job interleave whose worst case is a few seconds of mixed file rows that the re-enqueue path heals on its own.

Keep:

  • prFilesJobId helper in packages/das/src/queue/constants.ts and the expectedHeadSha/expectedBaseSha threading through pull-request.handler.ts, fetch.processor.ts, and the backfill enqueue.
  • The head/base WHERE-clause guard on the final scoringDataStored=true update in fetch.processor.ts (prGenerationCriteria).
  • handleStalePrFilesJob re-enqueue when the guarded update reports affected=0.

Drop:

  • writeIfCurrentPrFilesGeneration and the pessimistic_write transaction wrapper in github-fetcher.service.ts.
  • The isCurrentPrFilesGeneration checks sprinkled through fetchAndStorePrFiles and fetchAndStoreBatchedContents.
  • The contents-batching refactor — revert to the per-file upsert inside the loop.

Net result should be ~30 lines instead of ~330, same fix for the documented bug.

@MkDev11
Copy link
Copy Markdown
Author

MkDev11 commented May 10, 2026

done

@MKDev-ll
Copy link
Copy Markdown

MKDev-ll commented May 11, 2026

@anderdc please close my PR.
Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anderdc anderdc anderdc requested changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug] Active file-fetch jobs can mark stale PR contents complete after a newer push

3 participants

@MkDev11 @anderdc @MKDev-ll